Skip to content

[tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries#11616

Merged
dan-shaw merged 2 commits intomicrosoft:masterfrom
orudge:tinyxml2-fixed-exports
Jun 12, 2020
Merged

[tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries#11616
dan-shaw merged 2 commits intomicrosoft:masterfrom
orudge:tinyxml2-fixed-exports

Conversation

@orudge
Copy link
Contributor

@orudge orudge commented May 27, 2020

This PR updates tinyxml2 to version 8.0.0. It also adds a patch to prevent symbols being exported from DLLs/EXEs linked with the static version of the library. (This patch has been submitted to leethomason/tinyxml2#820 and hopefully will make it into the next release, so should only be temporary in vcpkg.)

I've tested builds of x86-windows, x86-windows-static, x64-windows, x64-windows-static, arm64-windows, arm64-windows-static. Symbols are still exported from the -windows DLLs correctly, and linked output of -windows-static no longer includes tinyxml2 symbols. Also tested x64-linux which looks to be fine.

@NancyLi1013 NancyLi1013 self-assigned this May 28, 2020
NancyLi1013
NancyLi1013 suggested changes May 28, 2020
View reviewed changes
@NancyLi1013 NancyLi1013 added category:port-update The issue is with a library, which is requesting update new revision requires:author-response labels May 28, 2020
@orudge orudge force-pushed the tinyxml2-fixed-exports branch from c8f29bc to 89ace06 Compare May 28, 2020 13:05
@NancyLi1013
Copy link
Contributor

NancyLi1013 commented Jun 1, 2020

@orudge
Could you please look into the regression on osx?

CMake Error in src/CMakeLists.txt:
  Imported target "TINYXML2::TINYXML2" includes non-existent path

    "/Volumes/data/work/1/s/packages/tinyxml2_x64-osx/debug/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

@orudge
Copy link
Contributor Author

orudge commented Jun 1, 2020

Could you please look into the regression on osx?

From what I can tell, this doesn't seem to be a regression (at least, not caused by my changes). Using a clean clone of Microsoft/vcpkg, with existing tinyxml2, I get the same error:

$ ./vcpkg install ignition-msgs5
Computing installation plan...
The following packages will be built and installed:
  * eigen3[core]:x64-osx
  * ignition-cmake2[core]:x64-osx
  * ignition-math6[core]:x64-osx
  * ignition-modularscripts[core]:x64-osx
    ignition-msgs5[core]:x64-osx
  * protobuf[core]:x64-osx
  * tinyxml2[core]:x64-osx

...

Starting package 7/7: ignition-msgs5:x64-osx
Building package ignition-msgs5[core]:x64-osx...
-- Downloading https://github.com/ignitionrobotics/ign-msgs/archive/ignition-msgs5_5.1.0.tar.gz...
-- Extracting source /Users/orudge/vcpkg/vcpkg/downloads/ignitionrobotics-ign-msgs-ignition-msgs5_5.1.0.tar.gz
-- Applying patch 01-protobuf.patch
-- Using source at /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/src/sgs5_5.1.0-8f48d4cbaf
-- Configuring x64-osx-dbg
CMake Error at scripts/cmake/vcpkg_execute_required_process.cmake:72 (message):
    Command failed: /Users/orudge/vcpkg/vcpkg/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/src/sgs5_5.1.0-8f48d4cbaf -DBUILD_TESTING=OFF -DCMAKE_MAKE_PROGRAM=/Users/orudge/vcpkg/vcpkg/downloads/tools/ninja-1.10.0-osx/ninja -DCMAKE_SYSTEM_NAME=Darwin -DBUILD_SHARED_LIBS=OFF -DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=/Users/orudge/vcpkg/vcpkg/scripts/toolchains/osx.cmake -DVCPKG_TARGET_TRIPLET=x64-osx -DVCPKG_SET_CHARSET_FLAG=ON -DVCPKG_PLATFORM_TOOLSET=external -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON -DCMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_SKIP=TRUE -DCMAKE_VERBOSE_MAKEFILE=ON -DVCPKG_APPLOCAL_DEPS=OFF -DCMAKE_TOOLCHAIN_FILE=/Users/orudge/vcpkg/vcpkg/scripts/buildsystems/vcpkg.cmake -DCMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION=ON -DVCPKG_CXX_FLAGS= -DVCPKG_CXX_FLAGS_RELEASE= -DVCPKG_CXX_FLAGS_DEBUG= -DVCPKG_C_FLAGS= -DVCPKG_C_FLAGS_RELEASE= -DVCPKG_C_FLAGS_DEBUG= -DVCPKG_CRT_LINKAGE=dynamic -DVCPKG_LINKER_FLAGS= -DVCPKG_TARGET_ARCHITECTURE=x64 -DCMAKE_INSTALL_LIBDIR:STRING=lib -DCMAKE_INSTALL_BINDIR:STRING=bin -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/Users/orudge/vcpkg/vcpkg/packages/ignition-msgs5_x64-osx/debug
    Working Directory: /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/x64-osx-dbg
    Error code: 1
    See logs for more information:
      /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/config-x64-osx-dbg-out.log
      /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/config-x64-osx-dbg-err.log

Call Stack (most recent call first):
  scripts/cmake/vcpkg_configure_cmake.cmake:306 (vcpkg_execute_required_process)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:3 (vcpkg_configure_cmake)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:131 (ignition_modular_build_library)
  ports/ignition-msgs5/portfile.cmake:13 (ignition_modular_library)
  scripts/ports.cmake:90 (include)


Error: Building package ignition-msgs5:x64-osx failed with: BUILD_FAILED
Please ensure you're using the latest portfiles with `.\vcpkg update`, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
  Package: ignition-msgs5:x64-osx
  Vcpkg version: 2020.02.04-unknownhash

It looks like the ignition-msgs5 port is looking for a non-existent vcpkg/packages/tinyxml2_x64-osx/debug/include folder - no other packages have a debug/include folder though, and I'm not quite sure where this is coming from for tinyxml2. Unfortunately I don't think this is something I have the time (or necessarily expertise) to debug further.

@traversaro
Copy link
Contributor

traversaro commented Jun 1, 2020

I think that at some point the CI machine has been modified to contain also the pkg-config command, and ignition-msgs5's by default uses pkg-config to find TinyXML2, and then uses a normal CMake-based search as a fallback. The tinyxml2 port has been installing faulty .pc files (see #10039), but as pkg-config was not installed in the CI machine, this problem was hidden. A possible workarond is either to fix #10039, or to modify the tinyxml2 port do not install .pc files until #10039 is fixed.

@orudge
Copy link
Contributor Author

orudge commented Jun 1, 2020

Thanks, that helps - the change I've pushed should fix #10039 too (via a patch).

@orudge orudge force-pushed the tinyxml2-fixed-exports branch from fd99a55 to 4dc579b Compare June 1, 2020 20:24
@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jun 4, 2020
@NancyLi1013
Copy link
Contributor

NancyLi1013 commented Jun 4, 2020

Just need to rerun this PR once the regressions are fixed in this PR #11742.

@NancyLi1013 NancyLi1013 removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jun 4, 2020
@NancyLi1013
Copy link
Contributor

NancyLi1013 commented Jun 8, 2020

@orudge
It seems that there are some new regressions generated. Could you please take a look?

@orudge orudge force-pushed the tinyxml2-fixed-exports branch from 4dc579b to 7e4a581 Compare June 8, 2020 10:40
@orudge
Copy link
Contributor Author

orudge commented Jun 8, 2020

It seems that there are some new regressions generated. Could you please take a look?

Most of these seem to be unrelated issues, primarily download errors or out of disk space (llvm). I can't find any issues relating to tinyxml2. I've rebased and pushed the changes again and it looks like the tests have failed again with a download error for a port.

@JackBoosY
Copy link
Contributor

JackBoosY commented Jun 9, 2020

/azp run

@JackBoosY JackBoosY requested a review from NancyLi1013 June 9, 2020 07:07
@azure-pipelines
Copy link

azure-pipelines bot commented Jun 9, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jun 10, 2020
@dan-shaw dan-shaw merged commit 50deb3e into microsoft:master Jun 12, 2020
@dan-shaw
Copy link
Contributor

dan-shaw commented Jun 12, 2020

Thanks for the PR!

@NancyLi1013 NancyLi1013 mentioned this pull request Jun 15, 2020
Closed
JangBoo pushed a commit to JangBoo/vcpkg that referenced this pull request Jun 18, 2020
@orudge @JangBoo
[tinyxml2] Update to 8.0.0; avoid exporting symbols when building sta…
7636f79 
…tic libraries (microsoft#11616)

* [tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries

* [tinyxml2] Clean up files
penumbra23 pushed a commit to codespace-dev/vcpkg that referenced this pull request Aug 5, 2020
@orudge @penumbra23
[tinyxml2] Update to 8.0.0; avoid exporting symbols when building sta…
a92abdc 
…tic libraries (microsoft#11616)

* [tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries

* [tinyxml2] Clean up files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@NancyLi1013 NancyLi1013

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@orudge @NancyLi1013 @traversaro @JackBoosY @dan-shaw